Repository navigation
Conversation
There was a problem hiding this comment.
Thanks for looking into bounding active formatting element reconstruction.
There are a few issues with the current approach that need to be addressed before this can land. Most importantly, throwing an uncaught error from afereconstruct() breaks the HTML5 parsing contract (where innerHTML and createDocument never throw on malformed markup) and causes Angular SSR requests to fail with an unhandled exception on small inputs, including non-malicious markup like lists with <i class="icon-..." /> tags. There are also a couple of ways the budget can be bypassed via non-string inputs, attribute amplification, or trailing padding.
See the inline comments for details.
| var input_char_count = 0; | ||
| var afe_reconstruction_count = 0; | ||
| var MIN_AFE_RECONSTRUCTION_BUDGET = 4096; | ||
| var AFE_RECONSTRUCTIONS_PER_INPUT_CHAR = 2; |
There was a problem hiding this comment.
A couple of issues with the max(4096, input_char_count * 2) threshold:
In HTML5, <i> is not a void element, so <i class="icon-0" /> leaves <i> open and pushes it onto afe.list. A normal list of 150 items like <ul><li><i class="icon-0" /> Item 0</li>...</ul> (~5.4 KB) triggers 11,175 reconstructions against a budget of 10,798 and throws. Even with zero attributes (which is allowed by the Noah's Ark clause across different formatting tags), 9 unclosed tags <p><b><i><u><s><em><strong><code><small><tt> followed by <p>a repeated 500 times (2 KB of linear HTML) performs 2.25 reconstructions per character and throws.
Also, createDocument(html) adds the full document length to input_char_count upfront, whereas createIncrementalHTMLParser().write(chunk) only counts chunks passed so far, so the same document can succeed in createDocument and throw when parsed in chunks.
| // Count only newly supplied input. Buffered leftovers are already | ||
| // accounted for, while process() and resume() re-enter with an empty | ||
| // string and therefore do not count the same input twice. | ||
| input_char_count += s.length; |
There was a problem hiding this comment.
s is not guaranteed to be a primitive string in all callers of parse(). While innerHTML coerces with String(v), domino.createDocument(html), domino.createWindow(html), and incrParser.end(s) pass html || '' directly. If a caller passes an object with a .toString() method and no numeric .length, s.length is undefined, which turns input_char_count into NaN. Later s += "\uFFFF" coerces s via .toString() and parses it, while afe_reconstruction_count > NaN is always false, bypassing the limit completely.
Also, because s.length is added upfront before tokenizing, appending 185 KB of trailing whitespace or an HTML comment to a 15 KB payload immediately raises reconstruction_budget to 400,000 reconstructions.
| afe_reconstruction_count++; | ||
| var reconstruction_budget = Math.max( | ||
| MIN_AFE_RECONSTRUCTION_BUDGET, | ||
| input_char_count * AFE_RECONSTRUCTIONS_PER_INPUT_CHAR | ||
| ); | ||
| if (afe_reconstruction_count > reconstruction_budget) { | ||
| throw new Error('HTML parser active formatting reconstruction limit exceeded'); | ||
| } |
There was a problem hiding this comment.
HTML5 parsing (innerHTML, outerHTML, insertAdjacentHTML, createDocument) is not supposed to throw on malformed markup (lib/HTMLParser.js:5303), and callers like Angular's _sanitizeHtml and SSR renderer do not wrap innerHTML in a try/catch. Throwing here turns a 1.4 KB input into an unhandled exception that fails the entire SSR request (and also skips reentrant_invocations-- in parse(), leaving the parser wedged if caught). Instead of throwing, it would be much safer to degrade gracefully by pruning stale entries from afe or capping afe.list capacity.
Also note that afeclone(doc, i) clones all afe.attrs[i].length attributes on every reconstruction. Counting 1 per element means an 8 KB input with <p><b a0 ... a1499></p> followed by 750 <p>a tags only reaches afe_reconstruction_count = 750 (well below 4,096), while still allocating over 1.1 million Attr objects.
| 'allows normally nested formatting elements': function() { | ||
| var doc = domino.createDocument(); | ||
| var template = doc.createElement('template'); | ||
| var html = ''; | ||
|
|
||
| for (var i = 0; i < 100; i++) { | ||
| html += '<p><b a' + i + '></b></p>'; | ||
| } | ||
|
|
||
| assert.doesNotThrow(function() { |
There was a problem hiding this comment.
In this test, '<p><b a' + i + '></b></p>' closes </b> before </p>, so adoptionAgency('b') removes <b> from afe.list on every iteration. As a result, afe.list is always empty when afereconstruct() runs, and afe_reconstruction_count stays 0 throughout the entire test. To exercise afereconstruct() on valid recovery markup, the formatting element needs to remain unclosed across </p>.
Summary
Bounds active formatting element reconstruction work during HTML parsing to prevent pathological mis-nested formatting markup from causing super-linear DOM growth and exhausting the Node.js heap.
This affects DOMino's HTML parsing path used by Angular server-side rendering.
Problem
When formatting elements are repeatedly left in the active formatting elements list,
afereconstruct()can reconstruct an increasingly large number of stale entries.A comparatively small input can therefore cause quadratic DOM allocation.
On current
main, using Node.js 22.22.3 with a 128 MiB heap limit, an 8,290-byte non-numeric test case causes the unpatched parser to terminate with a fatal V8 out-of-memory error (SIGABRT).This remains reproducible after the existing numeric attribute storage hardening (
c5aa1eb). The reproducer uses non-numeric attribute names, so the active-formatting reconstruction issue does not depend on sparse numeric attribute storage.Fix
Track:
Reconstruction work is bounded to:
If this limit is exceeded, parsing stops with a controlled error instead of continuing until the process exhausts its heap.
The accounting also handles incremental parsing: buffered input is counted when originally supplied, while
process()andresume()re-enter the parser with an empty string and do not count the same input again.Compatibility
The existing DOMino test corpus was also tested with substantially stricter experimental budgets.
All existing tests still passed with:
The submitted
max(4096, inputLength * 2)limit therefore provides additional compatibility headroom.Incremental parsing was additionally checked using chunk sizes of:
The pathological input was bounded in every case, while normally nested formatting continued to parse successfully.
Tests
Added regression coverage for:
The pathological regression fails before the fix with a missing expected exception and passes after applying the fix.
Full test suite on current
mainwith the patch:fixes : angular/angular#70926